Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP 8: Code Review Glossary #4136

Merged

Conversation

mjansenDatabay
Copy link
Contributor

@mjansenDatabay mjansenDatabay commented Mar 16, 2022

Hi @alex40724 ,

I completed the review of the Modules/Glossary component.

Results:

  • The code style is PSR-2 + X compliant
  • No usage of $_GET / $_POST / $_REQUEST / $_SESSION
  • There is a Unit Test Suite with at least one unit test
  • The unit tests pass
  • External libraries are compatible with PHP 8 (if relevant)
  • There are no serious PhpStorm Code Inspection issues left
  • The types are fully documented (PhpDoc) or explict PHP types are used (type hints, return types, typed properties)

I also fixed some trivial inspection profile issues.

The following methods MUST be checked:

  • \ilObjGlossary::create: An untyped boolean parameter is defined, the parent method has no parameter at all

Further actions needed:

Type documention or explict types are missing for the following methods:

  • \ilObjGlossary::getAdvMDSubItemTitle / Reason: No types defined/documented in parent class (which has to be provided by another maintainer)

I suggest to specify the expected type of arrays (where possible) by using PHPDoc, e.g.:

  • \ilObjGlossary::removeOfflineGlossaries: $a_glo_ids could be int[].
  • \ilObjGlossaryAccess::_lookupOnlineStatus: $a_ids could be int[], the return type is array<int, bool>.
  • ... some others ...

Best regards,
@mjansenDatabay

@mjansenDatabay mjansenDatabay added improvement php Pull requests that update Php code labels Mar 16, 2022
@mjansenDatabay mjansenDatabay force-pushed the review/8/php8-glossary branch from 2fea638 to 234c785 Compare March 16, 2022 12:31
@mjansenDatabay mjansenDatabay marked this pull request as ready for review March 16, 2022 12:39
@alex40724 alex40724 merged commit f0eda34 into ILIAS-eLearning:trunk Mar 17, 2022
@alex40724
Copy link
Member

@mjansenDatabay The first and last link you provided above are identical. I fixed the "Must" and "actions needed" issues.

@mjansenDatabay mjansenDatabay deleted the review/8/php8-glossary branch January 9, 2023 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants